Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(page_main_container): Remove fill container and min-height/width when fillable=FALSE #1188

Merged
merged 12 commits into from
Mar 5, 2025

Conversation

gadenbuie
Copy link
Member

@gadenbuie gadenbuie commented Mar 4, 2025

  • page_main_container() is only a fill carrier when fillable = TRUE. Or more specifically, it should only be a fill container when fillable is TRUE.
  • Only apply min-height/width when the main container is fillable.
library(shiny)
pkgload::load_all()
library(shinychat)

ui <- page_sidebar(
  actionButton("btn", "Button"),
  chat_ui("chat", fill = TRUE),
  fillable = TRUE
)

# ui <- page_navbar(
#   fillable = FALSE,
#   sidebar = sidebar(title = "Sidebar"),
#   nav_panel("one", actionButton("btn", "Button"), chat_ui("chat", fill = TRUE))
# )

server <- function(input, output, session) {
  observeEvent(input$chat_user_input, {
    # In a real app, this would call out to a chat model or API,
    # perhaps using the 'elmer' package.
    response <- paste0(
      "You said:\n\n",
      "<blockquote>",
      htmltools::htmlEscape(input$chat_user_input),
      "</blockquote>"
    )
    chat_append("chat", response)
  })
}

shinyApp(ui, server)

@gadenbuie gadenbuie requested a review from cpsievert March 4, 2025 17:25
R/page.R Outdated
tags$main(
class = "bslib-page-main bslib-gap-spacing",
...
)
),
item = TRUE,
Copy link
Collaborator

@cpsievert cpsievert Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably missing something here, but if the parent of bslib-page-main is not fillable, then AFAIK this doesn't need to be a fill item?

Suggested change
item = TRUE,
item = fillable,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not? But I know for sure that, without having to consider parent containers, we want to break fillability for children by making sure the container isn't a fill container. We could probably also set item = fillable, but then we have to consider all of the parent layouts. page_main_container() isn't just used in page_sidebar(), it's also used in navbar_page() with a sidebar where the DOM structure is quite different from page_sidebar().

I know for certain that removing html-fill-container does what we want it to do in this case and that that's correct in all layouts. That's also why I used .html-fill-container for min-height/min-width above. My general take is that .html-fill-item is upward-indicating, as in it makes the child filling when the parent context is fillable. .html-fill-container is downward-facing and communicates intent downward to its children.

My take is that we could probably either make .bslib-page-main a fill carrier when fillable = TRUE or add no filling properties when fillable = FALSE, but we could certainly work with the fill container property. So this choice mostly came down to 1) proving we want container = fillable is easy; 2) proving we want item = fillable requires enumerating all the layouts and making sure that's right.

Copy link
Collaborator

@cpsievert cpsievert Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, I was missing that navbar_page() context, thanks for writing that out. Although I'll probably find the code confusing to revisit, I like the careful approach your taking. Maybe consider linking to your comment here in the code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in the mean time I convinced myself that we should go wholesale as_fill_carrier() when fillable=TRUE and just wrap things in the <main> tag otherwise.

I think the reason I used as_fill_carrier() was in case we use page_main_container() in other situations; the fact that that also adds display: flex was the unintended side-effect.

Copy link
Collaborator

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add a NEWS.md, thanks!

@gadenbuie gadenbuie merged commit b7a02e7 into main Mar 5, 2025
1 check passed
@gadenbuie gadenbuie deleted the fix/page-main-container-fillable branch March 5, 2025 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants